media: Wire URL player duration and buffered ranges for tvOS - #11548
media: Wire URL player duration and buffered ranges for tvOS#11548abhijeetk wants to merge 2 commits into
Conversation
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
|
cc : @Gyuyoung @jkim-julie @rakuco |
There was a problem hiding this comment.
Code Review
This pull request introduces support for a URL-based player (URL player) on iOS/tvOS platforms using Starboard media, adding UrlPlayerDemuxer and UrlPlayerDemuxerStream as placeholders, and integrating them with StarboardRenderer and StarboardRendererClient. The review feedback recommends adding class comments to the new classes per the style guide, adding defensive null checks for player_bridge_ to prevent potential crashes, using DCHECK instead of CHECK for sequence verification, and aligning preprocessor guards to BUILDFLAG(IS_IOS_TVOS) && BUILDFLAG(USE_STARBOARD_MEDIA) to avoid compilation failures in non-Starboard configurations.
Add URL player support to StarboardRenderer for native HLS playback. UrlPlayerDemuxer provides placeholder streams for pipeline init while the platform player handles actual playback via SetSourceUrl. Bug: 512045535
through StarboardRenderer, StarboardRendererWrapper, Mojo IPC, and UrlPlayerDemuxer. This ensures that the JavaScript video.duration and video.buffered APIs correctly reflect the underlying platform player state on tvOS. Duration is reported once when the player reaches the presenting state. Buffered ranges are polled via GetMediaTime on each timer tick to provide continuous updates to the web application. Blocked by youtube#10698 Bug: 512045535
995ec4c to
4d11882
Compare
| #include "media/base/media_switches.h" | ||
| #include "media/base/renderer_factory.h" | ||
| #include "media/base/starboard/experimental_features.h" | ||
| #include "media/media_buildflags.h" |
There was a problem hiding this comment.
I don't think including this is necessary.
There was a problem hiding this comment.
The comments of this PR should be applied to #10698.
| #include "third_party/blink/public/web/web_view.h" | ||
| #include "ui/gfx/geometry/size_conversions.h" | ||
|
|
||
| #if BUILDFLAG(IS_IOS_TVOS) && BUILDFLAG(USE_STARBOARD_MEDIA) |
There was a problem hiding this comment.
USE_STARBOARD_MEDIA is true for Cobalt build anyway, so this is not necessary.
| kFrameInjectingDemuxer = 5, | ||
| kStreamProviderDemuxer = 6, | ||
| kManifestDemuxer = 7, | ||
| kUrlPlayerDemuxer = 8, // URL player placeholder demuxer. |
There was a problem hiding this comment.
nit: gate it by BUILDFLAG(USE_STARBOARD_MEDIA).
| #include "media/base/media_resource.h" | ||
|
|
||
| #include "base/no_destructor.h" | ||
| #include "media/media_buildflags.h" |
There was a problem hiding this comment.
This is not necessary, please check it.
| #include "media/base/demuxer_stream.h" | ||
| #include "media/base/media_export.h" | ||
| #include "build/build_config.h" | ||
| #include "media/media_buildflags.h" |
There was a problem hiding this comment.
Same for all #include "media/media_buildflags.h"
|
|
||
| if (base::FeatureList::IsEnabled(kCobaltBypassMojoForMedia) || | ||
| bypass_mojo_for_media_) { | ||
| // URL player is incompatible with the bypass bridge because the bypass |
There was a problem hiding this comment.
You can just gate the code by !BUILDFLAG(IS_IOS_TVOS)
There was a problem hiding this comment.
Thanks for the suggestion!
Gating the bypass bridge block with !BUILDFLAG(IS_IOS_TVOS) would also require gating the related members (bypass_mojo_for_media_, bypass_bridge_, bypass_id_), the static g_next_bypass_id, and the three helper methogs (OnTimeUpdateFromBridge, OnStatisticsUpdateFromBridge OnExtensionBypassInitialized), since they become unused on tvOS and trigger -Werror compile errors.
Let me know if you'd prefer a different approach here.
| void PaintVideoHoleFrame(const gfx::Size& size) override; | ||
| void UpdateStarboardRenderingMode(const StarboardRenderingMode mode) override; | ||
| void GetSbWindowHandle() override; | ||
| void OnDurationChange(base::TimeDelta duration) override; |
There was a problem hiding this comment.
As this is only used on tvOS, consider to gate them by BUILDFLAG(IS_IOS_TVOS), so it would override on other platforms. Same for OnBufferedTimeRangesChange.
| kFrameInjectingDemuxer = 5, | ||
| kStreamProviderDemuxer = 6, | ||
| kManifestDemuxer = 7, | ||
| kUrlPlayerDemuxer = 8, // URL player placeholder demuxer. |
There was a problem hiding this comment.
is_ios_tvos / is_tvos isn't available as a mojom enabled feature (only is_ios is registered in mojom.gni). Would [EnableIf=is_ios] work as a substitute since it's the closest available flag?
Currently at GN level, we identify tvos code using is_ios && target_platform == "tvos" but we can not use compound conditions with [EnableIf=...]
Please see our previous notes here regarding this constraint.
Aslo, media_types.mojom must stay in sync with demuxer.h and media_types_enum_mojom_traits.h.
The traits file has a switch that maps between ::media::DemuxerType::kUrlPlayerDemuxer (from
demuxer.h) and media::mojom::DemuxerType::kUrlPlayerDemuxer (from generated mojom). If one exists but the other doesn't, compilation fails.
As per your suggestion on demuxer.h to use BUILDFLAG(USE_STARBOARD_MEDIA), it means we need to use [EnableIf=use_starboard_media] here ?
| GetSbWindowHandle(); | ||
|
|
||
| // URL player duration and buffered ranges forwarding. | ||
| OnDurationChange(mojo_base.mojom.TimeDelta duration); |
There was a problem hiding this comment.
Please gate it both by use_starboard_media and is_ios_tvos
There was a problem hiding this comment.
As discussed above on media_types.mojom, is_ios_tvos isn't available as a mojom enabled feature.
The interface StarboardRendererClientExtension is already guarded by use_starboard_media, should we use [EnableIf=is_ios] here ?
| InitializeWithBypassBridge(uint32 bypass_bridge_id) => (bool success); | ||
|
|
||
| // Pass source URL for URL player. | ||
| SetSourceUrl(string source_url); |
There was a problem hiding this comment.
As discussed above on media_types.mojom, is_ios_tvos isn't available as a mojom enabled feature.
The interface StarboardRendererExtension is already guarded by use_starboard_media, should we use [EnableIf=is_ios] here ?
Plumb duration and buffered time range reporting from the URL player
through StarboardRenderer, StarboardRendererWrapper, Mojo IPC, and
UrlPlayerDemuxer. This ensures that the JavaScript video.duration and
video.buffered APIs correctly reflect the underlying platform player
state on tvOS.
Duration is reported once when the player reaches the presenting state.
Buffered ranges are polled via GetMediaTime on each timer tick to
provide continuous updates to the web application.
Blocked by #10698
Bug: 512045535